Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

TASK: document and assert Node.nodeName behavior #4469

Merged
merged 3 commits into from
Feb 8, 2024
Merged

TASK: document and assert Node.nodeName behavior #4469

merged 3 commits into from
Feb 8, 2024

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Sep 2, 2023 •
edited
Loading

related #4311

When looking into the php node readmodel it should be super clear that a nodename is guaranteed to be set if its tethered.

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign
Copy link
Member Author

mhsdesign commented Sep 2, 2023 •
edited
Loading

Sadly phpstan doenst allow us to annotate this behavior ...

https://phpstan.org/writing-php-code/narrowing-types#custom-type-checking-functions-and-methods

And also the feature like in @return doesnt work here https://phpstan.org/blog/phpstan-1-6-0-with-conditional-return-types:

/** @var ($this->classification is TETHRED ? string : null) */

playground:
https://phpstan.org/r/7c491f32-89df-4b4b-a218-00c2b1889933

bwaidelich
bwaidelich reviewed Sep 8, 2023
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for defensive style (even though people will probably not instantiate this class manually) because it makes invalid states impossible.
In the long run we will hopefully be able to remove NodeName and NodeType from the Node read model, and can remove the checks then.

Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Node.php Outdated Show resolved Hide resolved
Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Node.php Outdated Show resolved Hide resolved
@mhsdesign mhsdesign force-pushed the task/documentAndAssertNodeNodeNameBehaviour branch from af8d6e3 to 9328330 Compare September 19, 2023 10:15
@mhsdesign mhsdesign changed the base branch from nodeTypeFallbackHandling to 9.0 September 19, 2023 10:15
bwaidelich
bwaidelich reviewed Sep 19, 2023
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just two mini tweaks, +1 apart from those

@mhsdesign
Copy link
Member Author

mhsdesign commented Sep 19, 2023 •
edited
Loading

Hmm but now our ProjectionIntegrityViolationDetection fails:

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature

Line 42 in d614486

When the event NodeAggregateWithNodeWasCreated was published with payload: 

001 Scenario: Create node variants of different type                             # Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature:41
      When the event NodeAggregateWithNodeWasCreated was published with payload: # Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature:42
        InvalidArgumentException: The NodeName must be set if the Node is tethered. in /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/Projection/ContentGraph/Node.php:79
        Stack trace:
        #0 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/NodeFactory.php(67): Neos\ContentRepository\Core\Projection\ContentGraph\Node->__construct()
        #1 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/NodeFactory.php(166): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory->mapNodeRowToNode()
        #2 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentGraph.DoctrineDbalAdapter/src/Domain/Repository/ContentGraph.php(245): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\NodeFactory->mapNodeRowsToNodeAggregate()
        #3 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Data/Temporary/Testing/SubContextBehat/Cache/Code/Flow_Object_Classes/Neos_Neos_Fusion_Cache_GraphProjectorCatchUpHookForCacheFlushing.php(176): Neos\ContentGraph\DoctrineDbalAdapter\Domain\Repository\ContentGraph->findNodeAggregateById()
        #4 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/Projection/DelegatingCatchUpHook.php(44): Neos\Neos\Fusion\Cache\GraphProjectorCatchUpHookForCacheFlushing_Original->onAfterEvent()
        #5 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/ContentRepository.php(183): Neos\ContentRepository\Core\Projection\DelegatingCatchUpHook->onAfterEvent()
        #6 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Libraries/neos/eventstore/src/CatchUp/CatchUp.php(148): Neos\ContentRepository\Core\ContentRepository->Neos\ContentRepository\Core\{closure}()
        #7 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/ContentRepository.php(192): Neos\EventStore\CatchUp\CatchUp->run()
        #8 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Data/Temporary/Testing/SubContextBehat/Cache/Code/Flow_Object_Classes/Neos_ContentRepositoryRegistry_Factory_ProjectionCatchUpTrigger_CatchUpTriggerWithSynchronousOption.php(70): Neos\ContentRepository\Core\ContentRepository->catchUpProjection()
        #9 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.Core/Classes/EventStore/EventPersister.php(70): Neos\ContentRepositoryRegistry\Factory\ProjectionCatchUpTrigger\CatchUpTriggerWithSynchronousOption_Original->triggerCatchUp()
        #10 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/GenericCommandExecutionAndEventPublication.php(145): Neos\ContentRepository\Core\EventStore\EventPersister->publishEvents()
        #11 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Neos/Neos.ContentRepository.TestSuite/Classes/Behavior/Features/Bootstrap/Features/NodeCreation.php(293): FeatureContext->publishEvent()
        #12 /home/runner/work/neos-development-collection/neos-development-collection/neos-development-distribution/Packages/Libraries/behat/behat/src/Behat/Testwork/Call/Handler/RuntimeCallHandler.php(110): FeatureContext->theEventNodeAggregateWithNodeWasCreatedWasPublishedToStreamWithPayload()

because of this (enabled) GraphProjectorCatchUpHookForCacheFlushing

neos-development-collection/Neos.Neos/Classes/Fusion/Cache/GraphProjectorCatchUpHookForCacheFlushing.php

Line 176 in 52be2f0

$nodeAggregate = $this->contentRepository->getContentGraph()->findNodeAggregateById(

A hack would be to disable it temporary? private static bool $enabled = true;

@bwaidelich
Copy link
Member

because of this (enabled)

What is the root cause? Can't we fix that (e.g. adjust the test such that it does not create an invalid node)?

@mhsdesign
Copy link
Member Author

What is the root cause? Can't we fix that (e.g. adjust the test such that it does not create an invalid node)?

as far as i understand is that we test that if a the database projection has an invalid node (somehow) we detect it.
For this to achieve we cannot use the normal commands but tell behat that those events are published (hacky! - but one of the few only ways, we should refrain from doing Given the event #4336)
That results in some catchups being triggered that wont work because something is really wrong.

@mhsdesign mhsdesign force-pushed the task/documentAndAssertNodeNodeNameBehaviour branch from 674ef8b to 1e6bf9d Compare November 12, 2023 16:31
@mhsdesign
Copy link
Member Author

yes after adjusting the catch up the tests pass.

The neos.neos catchup is registers itself here into the default preset

neos-development-collection/Neos.Neos/Configuration/Settings.ContentRepositoryRegistry.yaml

Lines 18 to 21 in cc4a0a6

'Neos.ContentRepository:ContentGraph':
catchUpHooks:
'Neos.Neos:FlushContentCache':
factoryObjectName: Neos\Neos\Fusion\Cache\GraphProjectorCatchUpHookForCacheFlushingFactory

and since we use the content repository registry in Neos.ContentRepository.BehavioralTests

neos-development-collection/Neos.ContentRepository.BehavioralTests/Tests/Behavior/Bootstrap/FeatureContext.php

Line 164 in a177872

$contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId);

we get everything registered for the default cr for free ;)

this is partially a good thing as we fully e2e test everything (at least that stuff generally works) but might also cause trouble like when doctrine is not setup but a projection needs it or it might slow down tests.

@mhsdesign mhsdesign force-pushed the task/documentAndAssertNodeNodeNameBehaviour branch from 1e6bf9d to 328e308 Compare February 1, 2024 20:46
@mhsdesign
TASK: Manipulate content graph manually for TetheredNodesAreNamed test
cd8408c 
otherwise it's basically impossible now to reproduce the state, as publishing an event will lead to the catchup's to run, which might instantiate a node:

the neos.neos `GraphProjectorCatchUpHookForCacheFlushingFactor` catchUpHook would fail for example:

> When the event NodeAggregateWithNodeWasCreated was published with payload:
> # Features/ProjectionIntegrityViolationDetection/TetheredNodesAreNamed.feature:41
> InvalidArgumentException: The NodeName must be set if the Node is tethered.
@mhsdesign mhsdesign force-pushed the task/documentAndAssertNodeNodeNameBehaviour branch from 328e308 to cd8408c Compare February 2, 2024 23:12
@mhsdesign
TASK: Add @var types to \TestSuite\GenericCommandExecutionAndEventPub…
1602b8d 
…lication::publishEvent

... where its impossible to infer types
@mhsdesign
Copy link
Member Author

Thanks for all your input today. Inspired from some other tests in the doctrine dbal adapter:

To modifying the db directly to provoke this invalid state, i moved the test over there and did the same ;)

see cd8408c

hopefully this can now be finally merged :D Didnt expect this to blow up that much :P

bwaidelich
bwaidelich approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one, thanks!

@mhsdesign mhsdesign merged commit ea9ff61 into 9.0 Feb 8, 2024
10 checks passed
@mhsdesign mhsdesign deleted the task/documentAndAssertNodeNodeNameBehaviour branch February 8, 2024 14:44
@mhsdesign
Copy link
Member Author

Thank you all for the extensive discussion last Friday (@kitsunet @bwaidelich @nezaniel) and sorry for hijacking that weekly partly for this non critical thing :D Im happy that this is older task of mine is now resolved ;)

@mhsdesign mhsdesign mentioned this pull request Jul 5, 2024
Closed
@mhsdesign mhsdesign mentioned this pull request Nov 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwaidelich bwaidelich bwaidelich approved these changes

@skurfuerst skurfuerst Awaiting requested review from skurfuerst

@nezaniel nezaniel Awaiting requested review from nezaniel

@ahaeslich ahaeslich Awaiting requested review from ahaeslich

@dlubitz dlubitz Awaiting requested review from dlubitz

Assignees
No one assigned
Labels
9.0beta2 9.0 Task
Projects
Neos 9.0 Release Board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mhsdesign @bwaidelich